Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix reading msvs version on Windows #2644

Merged
merged 2 commits into from
Jun 25, 2023
Merged

Fix reading msvs version on Windows #2644

merged 2 commits into from
Jun 25, 2023

Conversation

jgcook935
Copy link
Contributor

@jgcook935 jgcook935 commented Apr 13, 2022 •
edited
Loading

  • npm install && npm test passes
  • tests are included
  • documentation is changed or added
  • commit message follows commit guidelines
Description of change

Ever since #2547 went in for version 9 our Windows machines can't properly read msvs_version from the .npmrc. Since names with '_' are now changed to '-' we need to change the opts definition. If there's a different way you'd like to handle this let me know and I can update.

@jgcook935 jgcook935 force-pushed the msvs-config branch 2 times, most recently from 2ac9350 to cc58bcf Compare April 13, 2022 16:50
@jeremywho jeremywho mentioned this pull request Sep 12, 2022
Closed
@cclauss cclauss mentioned this pull request Mar 18, 2023
Open
@cclauss
Copy link
Contributor

cclauss commented May 25, 2023 •
edited
Loading

@StefanStojanovic Your review, please.

Also, what are the currently supported values for MSVS?

Why is this in our tests?

node-gyp/.github/workflows/tests.yml

Lines 46 to 47 in bb76021

echo 'GYP_MSVS_VERSION=2015' >> $Env:GITHUB_ENV
echo 'GYP_MSVS_OVERRIDE_PATH=C:\\Dummy' >> $Env:GITHUB_ENV

@cclauss cclauss changed the title Fix reading msvs version on windows Fix reading msvs version on Windows May 25, 2023
@StefanStojanovic StefanStojanovic mentioned this pull request Jun 2, 2023
Closed
@StefanStojanovic
Copy link
Contributor

Why is this in our tests?

node-gyp/.github/workflows/tests.yml

Lines 46 to 47 in bb76021

echo 'GYP_MSVS_VERSION=2015' >> $Env:GITHUB_ENV
echo 'GYP_MSVS_OVERRIDE_PATH=C:\\Dummy' >> $Env:GITHUB_ENV

I've made a draft PR to test this. From what I see, it is required for Python tests to pass. The same code exists in the visual-studio workflow file, but since Python tests are not run there it can be removed. By the way, the last change on that part of the code was done 3 years ago.

@StefanStojanovic
Copy link
Contributor

The change looks good. @jgcook935 could you rebase to the latest main (will require rewriting your test to mocha) so it can run through CI, please?

@StefanStojanovic
Copy link
Contributor

@jgcook935 I can go ahead and apply the changes I mentioned above and move this forward. Please let me know if you want to do it yourself. Thanks.

@StefanStojanovic
Copy link
Contributor

After rebasing and rewriting the test to mocha everything's good. I'm planning to land this on Friday unless someone objects before that.

@StefanStojanovic StefanStojanovic merged commit 53c99ae into nodejs:main Jun 25, 2023
lukekarrys pushed a commit that referenced this pull request Dec 2, 2024
@jgcook935 @lukekarrys
Fix reading msvs version on Windows (#2644)
4228b29 
* fix: fix reading msvs version on windows
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@StefanStojanovic StefanStojanovic StefanStojanovic approved these changes

Assignees
No one assigned
Labels
Windows
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jgcook935 @cclauss @StefanStojanovic